Skip to content

Accept and lower the accessor keyword in TypeScript files using experimentalDecorators - #38125

Open
robobun wants to merge 7 commits into
mainfrom
farm/ccdeb891/accessor-legacy-decorators
Open

robobun wants to merge 7 commits into
mainfrom
farm/ccdeb891/accessor-legacy-decorators

Conversation

@robobun

@robobun robobun commented Aug 13, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #29197

Replaces #29201 (closed) and the parked branch farm/57075cac/ts-legacy-decorators-accessor. #29201 routed these classes through the standard-decorator lowering and reported a compile error for a class that mixes legacy decorators with accessor members; tsc accepts that combination, so the in-place lowering here was kept. Its test file was run against this branch: every case passes except that deliberate error and a standard-mode (experimentalDecorators off) computed-key case, which belongs to #31926. The one case it covered that this suite lacked (TypeScript modifiers on accessors) was added here, together with the mixed class.

Problem

  • In a project whose tsconfig sets experimentalDecorators (or emitDecoratorMetadata), any .ts class using the accessor keyword fails to parse: class A { accessor name = "A" } gives error: Expected ";" but found "name". The same file works when the tsconfig does not enable legacy decorators. tsc accepts the keyword in both decorator modes (it is class syntax from the decorators proposal, independent of which decorator flavor is configured).
  • Cause: src/js_parser/parse/parse_property.rs only recognized the keyword when features.standard_decorators was set, and ParseTask.rs / transpiler.rs clear that flag for TypeScript files with experimentalDecorators or emitDecoratorMetadata. accessor then parsed as a field named accessor.
  • Parsing alone is not enough: JavaScriptCore does not implement accessor, and the only lowering for it lives in the standard-decorator path (lower_decorators.rs), which these classes never take because their decorators are TypeScript legacy decorators lowered by P::lower_class.
  • Adjacent bug, both modes: Class::can_be_moved (src/ast/g.rs) only looked at plain static fields, so a class whose only side effect is a static accessor initializer was hoisted to the top of the module by the runtime transpiler and read bindings before their declaration (ReferenceError: Cannot access 'names' before initialization). Reproduces on main with a .js file too.

Fix

  • parse_property.rs: recognize accessor in class bodies in every decorator mode. The newline (ASI) and raw-spelling checks are unchanged, so accessor as a member name still parses the way tsc parses it (covered by a snapshot test). In experimentalDecorators mode the abstract branch there turns a decorated abstract accessor x into the same Abstract property a decorated abstract x becomes, so its decorators are emitted as __legacyDecorateClassTS(decorators, proto, "x", undefined) plus design:type, which is what tsc emits; before, the member (and its decorators) was dropped like an undecorated abstract member. Standard-decorator mode is left as it was (the member is still dropped; tsc rejects standard decorators on abstract members), covered in es-decorators.test.ts.
  • New src/js_parser/lower/lower_auto_accessors.rs, called from visit_class for classes that should_lower_standard_decorators does not claim. Each accessor is replaced at its own position by the desugaring the proposal defines and tsc emits:
    accessor x = 1;   // -> #x = 1; get x() { return this.#x; } set x(v) { this.#x = v; }
    • Why in place: nothing leaves the class body, so key evaluation order, initializer order and private-name scoping are exactly those of the source, and it composes with the legacy decorator lowering that runs afterwards on the same class. Static accessors use this, like the proposal and the existing standard-mode output.
    • Why in visit_class, before the useDefineForClassFields: false block: the generated backing field is then relocated into the constructor in source order together with the other instance fields, which is what tsc emits for that option.
    • Backing name: #x (#_p for accessor #p, #_accessor_storage for other keys), with a numeric suffix when that spelling is declared by this class or by an enclosing class, found by walking the scope chain while the class body scope is still current. Private names are printed verbatim by the non-minifying renamer, and a name declared by an enclosing class may be referenced from inside this class body, so both sets have to be avoided.
    • Computed keys are evaluated once, at the accessor's position: get [_computedKey = expr]() {} set [_computedKey]() {}. The temporary comes from generate_temp_ref (collision-safe name in the runtime transpiler), is registered in the scope a var there hoists to, and is recorded as a top-level declared symbol of the part when that scope is the module scope, so the bundler's renamer gives it a name distinct from other files' top-level bindings. Without the last step, the temporary of an earlier file and a top-level binding of a later file could be assigned the same name (verified while writing the bundling test).
    • Legacy decorators on an accessor move to the generated setter. lower_class then emits __legacyDecorateClassTS(decorators, target, key, null), which is what tsc's __decorate receives for a legacy-decorated accessor (the decorator gets the get/set descriptor and can replace it). The setter rather than the getter because lower_class reuses the decorated member's key expression in that call: the setter's key is the bare _computedKey temporary, so a computed key is still evaluated once (tsc emits __decorate(..., _a, null) the same way). The pair is flagged IsLoweredAutoAccessor (new flags::Property variant) and the setter carries the member's declared type, so emitDecoratorMetadata emits design:type only, as tsc does for accessors, instead of a getter's or setter's design:type + design:paramtypes.
    • Standard-decorator mode is unchanged: those classes always have should_lower_standard_decorators set, so the new pass never runs for them (checked that bun build --no-bundle output for JS/TS files without experimentalDecorators is byte-identical before and after). Lower accessor-only classes in place instead of relocating static elements #31926, which makes the standard-mode lowering of accessor-only classes in place as well, is independent of this; the new pass is written so that path could share it later.
  • src/ast/g.rs: can_be_moved also inspects static accessor initializers. Because this changes the output of files that already transpiled before (the other changes only affect files that used to fail to parse), EXPECTED_VERSION in src/jsc/RuntimeTranspilerCache.rs is bumped to 26; the cache key is source hash plus features, not the bun version, so without the bump a warm cache would keep serving the hoisted output for such files until they are edited.
  • Small enablers: four helpers in lower_decorators.rs became pub(crate) for reuse; the obsolete accessor TODO in lower_class and the stale comment in emit_decorator_metadata_for_prop were removed.
  • Verification:
    • test/bundler/transpiler/decorators.test.ts, new describe("accessor keyword with experimentalDecorators"): output shapes via Bun.Transpiler (basic, static/private/literal/computed keys, name collisions incl. an enclosing class, decorated instance/static/computed-key accessors, private / protected readonly / public static / static override / abstract / decorated abstract modifiers in a class whose other members carry legacy decorators, metadata vs. a real getter, useDefineForClassFields: false, accessor as a plain member name) and runtime fixtures with a tsconfig on disk (getter/setter semantics incl. subclass override and super, computed key evaluated once, anonymous export default keeps .name === "default", legacy decorators receiving and replacing the descriptor plus design:type, including on a computed-key accessor whose key function must run exactly once, useDefineForClassFields: false ordering, and a two-file Bun.build where the temporaries, a user function and an export all want the same name). 11 of the 12 fail on the released build (parse error), all pass with this branch.
    • test/bundler/transpiler/es-decorators.test.ts: .js class statement and export default class with static accessor initializers are no longer hoisted above the binding they read; fails on the released build with the ReferenceError above.
    • decorators, decorator-metadata, es-decorators, es-decorators-esbuild, ts-use-define-for-class-fields, bundler_decorator_metadata, esbuild/ts, esbuild/lower, bundler_edgecase and the internal source lints pass with a debug build; cargo clippy -p bun_js_parser is clean.

Background

  • Auto-accessor: accessor x = v declares a getter/setter pair backed by a hidden per-instance slot; the proposal specifies it as exactly the #x + get/set desugaring above. TypeScript has accepted it since 4.9 under both experimentalDecorators and standard decorators, and its legacy decorator transform decorates such a member with the descriptor form (__decorate(..., key, null)), emitting only design:type as metadata.
  • Decorator modes in bun: .js files and .ts files without experimentalDecorators use the standard (TC39) lowering in lower_decorators.rs, which also lowers accessors; .ts files with experimentalDecorators / emitDecoratorMetadata have features.standard_decorators cleared and get their decorators lowered by P::lower_class into __legacyDecorateClassTS calls after the class. G::Class::should_lower_standard_decorators is what routes a class to the former.
  • useDefineForClassFields: false: tsc option under which instance field initializers are turned into this.x = init assignments in the constructor; bun implements it at the end of visit_class.
  • Renamers: the runtime transpiler prints symbol names verbatim (generated names must be unique by construction, which generate_temp_ref provides), the bundler renames top-level symbols across all files of a chunk and then nested scopes per part (so a symbol declared by a top-level var has to be registered as a top-level declaration to take part in the cross-file pass), and private names are never renamed except by the minifier.
  • Class::can_be_moved: the runtime transpiler hoists top-level classes without observable side effects to the top of the module to make some import cycles work; a static initializer is such a side effect.

[review] gate passed · iteration 0 · 11 files touched

fails on main (without fix)
ASAN without fix: 12 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/bundler/transpiler/decorators.test.ts test/bundler/transpiler/es-decorators.test.ts
bun test v1.4.1 (65362b53b)

test/bundler/transpiler/decorators.test.ts:
(pass) decorator order of evaluation [49.87ms]
(pass) decorator factories order of evaluation [42.99ms]
(pass) parameter decorators [32.11ms]
(pass) decorators random [71.12ms]
(pass) class field order [9.58ms]
(pass) changing static method [10.67ms]
(pass) class extending from another class [16.62ms]
(pass) decorated fields moving to constructor [10.21ms]
(pass) only class decorator [5.48ms]
(pass) decorators with different property key types [13.39ms]
(pass) only property decorators [13.46ms]
(pass) only argument decorators [5.95ms]
(pass) no decorators [5.01ms]
(pass) constructor statements > with parameter properties [14.59ms]
(pass) constructor statements > class expressions (no decorators) [13.47ms]
(pass) constructor statements > with parameter properties and statements [7.93ms]
(pass) constructor statements > with parameter properties, statements, and decorators [10.97
... (truncated)

release without fix: 12 FAILED
bun test v1.4.1-canary.1 (65362b53b)

test/bundler/transpiler/decorators.test.ts:
(pass) decorator order of evaluation [0.46ms]
(pass) decorator factories order of evaluation [0.41ms]
(pass) parameter decorators [0.40ms]
(pass) decorators random [0.96ms]
(pass) class field order [0.12ms]
(pass) changing static method [0.11ms]
(pass) class extending from another class [0.18ms]
(pass) decorated fields moving to constructor [0.11ms]
(pass) only class decorator [0.05ms]
(pass) decorators with different property key types [0.20ms]
(pass) only property decorators [0.17ms]
(pass) only argument decorators [0.06ms]
(pass) no decorators [0.04ms]
(pass) constructor statements > with parameter properties [0.13ms]
(pass) constructor statements > class expressions (no decorators) [0.12ms]
(pass) constructor statements > with parameter properties and statements [0.07ms]
(pass) constructor statements > with parameter properties, statements, and decorators [0.10ms]
(pass) constructor statements > with more parameter properties, statements, and decorators [0.17ms]
(pass) constructor statements > expression with parameter properties and statements [0.10ms]
(pass) export default class 
... (truncated)
passes on PR (with fix)
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/bundler/transpiler/decorators.test.ts test/bundler/transpiler/es-decorators.test.ts
bun test v1.4.1 (65362b53b)

test/bundler/transpiler/decorators.test.ts:
(pass) decorator order of evaluation [51.04ms]
(pass) decorator factories order of evaluation [39.84ms]
(pass) parameter decorators [30.21ms]
(pass) decorators random [66.84ms]
(pass) class field order [9.08ms]
(pass) changing static method [10.28ms]
(pass) class extending from another class [16.05ms]
(pass) decorated fields moving to constructor [9.51ms]
(pass) only class decorator [4.93ms]
(pass) decorators with different property key types [12.00ms]
(pass) only property decorators [11.36ms]
(pass) only argument decorators [5.53ms]
(pass) no decorators [4.32ms]
(pass) constructor statements > with parameter properties [12.95ms]
(pass) constructor statements > class expressions (no decorators) [12.28ms]
(pass) constructor statements > with parameter properties and statements [7.28ms]
(pass) constructor statements > with parameter properties, statements, and decorators [9.83ms
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped)
  target       linux-x64-gnu
  build type   Release
  build dir    ./build/release
  revision     e9b7b98728
  features     baseline

23 deps, 131 codegen, 1172 objects in 1260ms

ninja: Entering directory `/workspace/bun/build/release'
[1/1244] gen bindgenv2
[2/1244] install /workspace/bun
bun install v1.4.1-canary.1 (65362b53b)

Checked 26 installs across 63 packages (no changes) [24.00ms]
[3/1244] fetch zlib
[zlib] up to date
[4/1244] install /workspace/bun/packages/bun-error
bun install v1.4.1-canary.1 (65362b53b)

Checked 1 install across 2 packages (no changes) [1.00ms]
[5/1244] gen ProcessBindingConstants.lut.h
Generating /workspace/bun/build/release/codegen/ProcessBindingConstants.lut.h from /workspace/bun/src/jsc/bindings/ProcessBindingConstants.cpp
[6/1244] fetch libjpeg-turbo
[libjpeg-turbo] up to date
[7/1217] fetch tinycc
[tinycc] up to date
[8/1216] gen JSBuffer.lut.h
Generating /workspace/bun/build/release/codegen/JSBuffer.lut.h from /workspace/bun/src/jsc/bindings/JSBuffer.cpp
[9/1216] gen bake.{client,server,error}.js
-> bake.client.js, bake.server.js, bake.error.js
... (truncated)
diff hotspot
src/ast/g.rs                                  |   6 +-
 src/ast/lib.rs                                |   3 +
 src/js_parser/lower/lower_auto_accessors.rs   | 266 ++++++++++++
 src/js_parser/lower/lower_decorators.rs       |  10 +-
 src/js_parser/lower/mod.rs                    |   1 +
 src/js_parser/p.rs                            |  14 +-
 src/js_parser/parse/parse_property.rs         |  10 +-
 src/js_parser/visit/mod.rs                    |   6 +
 src/jsc/RuntimeTranspilerCache.rs             |   5 +-
 test/bundler/transpiler/decorators.test.ts    | 598 +++++++++++++++++++++++++-
 test/bundler/transpiler/es-decorators.test.ts |  37 ++
 11 files changed, 942 insertions(+), 14 deletions(-)

gate history · 4 passed · 0 rejected · iteration 0

evidence per changed file
file                                           reads  edits  tests
src/ast/g.rs                                       0      0      0
src/ast/lib.rs                                     0      0      0
src/js_parser/lower/lower_auto_accessors.rs        0      0      0
src/js_parser/lower/lower_decorators.rs            0      0      0
src/js_parser/lower/mod.rs                         0      0      0
src/js_parser/p.rs                                 0      0      0
src/js_parser/parse/parse_property.rs              3      2      0
src/js_parser/visit/mod.rs                         0      0      0
src/jsc/RuntimeTranspilerCache.rs                  2      2      0
test/bundler/transpiler/decorators.test.ts         0      0      0
test/bundler/transpiler/es-decorators.test.ts      1      2      0

root cause · written by the author bot

The parser did not support the accessor class-member keyword under TypeScript's experimentalDecorators mode, so auto-accessors were rejected or mishandled instead of being desugared like tsc does. The fix teaches the parser to accept accessor and adds a dedicated lowering pass that rewrites each accessor x = init into a private backing field with a generated getter and setter pair, attaching decorators and metadata to the setter so computed keys evaluate exactly once and only design:type is emitted. It also marks static accessor initializers as side effects in Class::can_be_moved …

@robobun

robobun commented Aug 13, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 3:06 AM PT - Aug 28th, 2026

❌ @robobun, your commit e9b7b98 has 1 failures in Build #107601 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 38125

That installs a local version of the PR into your bun-38125 executable, so you can run:

bun-38125 --bun

@coderabbitai

coderabbitai Bot commented Aug 13, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file.

Or wait 7 minutes for your next included review.

View limit details

Limit details: You’ve used all 5 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3275f516-5365-4a7e-a5ea-640352a41bf3

📥 Commits

Reviewing files that changed from the base of the PR and between 1beee7a and e9b7b98.

📒 Files selected for processing (11)
  • src/ast/g.rs
  • src/ast/lib.rs
  • src/js_parser/lower/lower_auto_accessors.rs
  • src/js_parser/lower/lower_decorators.rs
  • src/js_parser/lower/mod.rs
  • src/js_parser/p.rs
  • src/js_parser/parse/parse_property.rs
  • src/js_parser/visit/mod.rs
  • src/jsc/RuntimeTranspilerCache.rs
  • test/bundler/transpiler/decorators.test.ts
  • test/bundler/transpiler/es-decorators.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 13, 2026 •

Copy link
Copy Markdown
Collaborator Author

Reproduced on main with the released build: in a directory whose tsconfig.json has "experimentalDecorators": true, bun file.ts on class A { accessor name = "A" } fails with Expected ";" but found "name", and new Bun.Transpiler({ loader: "ts", tsconfig: { compilerOptions: { experimentalDecorators: true } } }).transformSync(...) throws the same parse error. The static-accessor hoisting case reproduces on a plain .js file as well.

Fix and tests are in this PR; the new tests in test/bundler/transpiler/decorators.test.ts and es-decorators.test.ts fail on the released build and pass with this branch.

Consolidated with #29201 (closed): its test file was run against this branch and every case passes except the compile error it deliberately emitted for classes mixing legacy decorators with accessor (tsc accepts those; class E { @dec() id = 0; accessor name = "" } now emits the getter/setter pair plus the usual __legacyDecorateClassTS call for id, the shape tsc 6.0 emits as well) and one standard-mode computed-key case that #31926 fixes. The modifier coverage it had (private / protected readonly / public static / static override / abstract accessor) was added to the describe block here in 11227cb.

Follow-ups from comparing more shapes against tsc 6.0 on this branch: a decorated abstract accessor lost its decorators in experimentalDecorators mode (tsc emits the __decorate call), fixed in 4e3f4b1 and scoped to that mode in 04aa1be; and since the can_be_moved change alters the output of files that transpiled fine before, the runtime transpiler cache version is bumped in c5be06f. Everything else probed (nested classes and class expressions with computed keys, class decorators combined with decorated accessors, useDefineForClassFields: false with computed keys, minified bundling, ambient and abstract classes) matched the expected shapes; the one remaining difference from tsc is documented in the description (static accessors read the storage through this, as the proposal and the standard-mode lowering do, where tsc uses the class binding).

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. Parse and lower accessor fields under experimentalDecorators #29201 - Fixes the same issue (Decorators on accessor class fields fail to parse in Bun #29197) at the same three sites: the standard_decorators gate in parse_property.rs, Class::can_be_moved for static accessor initializers, and the computed-key handling in lower_decorators.rs.
  2. Lower accessor-only classes in place instead of relocating static elements #31926 - Adds lower_auto_accessors_in_place implementing the identical accessor x = 1 → #x = 1; get x(){} set x(v){} desugaring, with the same backing-name uniqueness scheme and evaluate-computed-key-once trick, on the standard-decorator path.
  3. js_parser: lower undecorated auto-accessors to a native #-private storage field #35708 - A third implementation of the same auto-accessor → #-private storage + get/set lowering, including the same computed-key capture and private-name collision avoidance, scoped to decorated classes in lower_decorators.rs.

🤖 Generated with Claude Code

@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Not duplicates, for the record:

Comment thread src/js_parser/lower/lower_auto_accessors.rs
Comment thread src/ast/lib.rs Outdated
Comment thread src/js_parser/lower/lower_auto_accessors.rs Outdated
Comment thread src/js_parser/lower/lower_auto_accessors.rs Outdated
Comment thread src/js_parser/lower/lower_auto_accessors.rs Outdated
Comment thread src/js_parser/lower/lower_auto_accessors.rs Outdated
Comment thread src/js_parser/lower/lower_auto_accessors.rs Outdated
Comment thread src/js_parser/lower/lower_auto_accessors.rs Outdated
Comment thread src/js_parser/p.rs Outdated
Comment thread src/js_parser/p.rs Outdated
Comment thread src/js_parser/parse/parse_property.rs Outdated
Comment thread src/js_parser/visit/mod.rs Outdated
Comment thread src/ast/lib.rs
Comment thread src/js_parser/lower/lower_auto_accessors.rs
Comment thread src/js_parser/lower/lower_auto_accessors.rs
Comment thread src/js_parser/lower/lower_auto_accessors.rs
Comment thread src/js_parser/lower/lower_auto_accessors.rs
Comment thread src/js_parser/lower/lower_auto_accessors.rs
Comment thread src/js_parser/parse/parse_property.rs
Comment thread src/js_parser/visit/mod.rs

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-reviewed after 460ddf4 and 3a37487 — the double-evaluation of a decorated computed accessor key I flagged earlier is fixed (decorators now ride on the setter, and the new runtime test asserts keyEvaluations: 1 with the decorator seeing k0). No further issues found, but this adds a new lowering pass that interacts with the legacy-decorator lowering, useDefineForClassFields, the bundler renamer and metadata emission, so a human look is still worthwhile.

What was reviewed:

  • lower_auto_accessors_in_place: private-name uniqueness across enclosing scopes, computed-key temp declaration in the hoisting scope with declared_symbols registration, and placement before the useDefineForClassFields block.
  • can_be_moved now inspects static accessor initializers — checked that the standard-decorator path still routes those classes through should_lower_standard_decorators so this pass never runs for them.
  • The new IsLoweredAutoAccessor metadata arm emits only design:type (matching tsc), and ts_metadata on the setter carries the declared type.
Extended reasoning...

Overview

The PR removes the features.standard_decorators gate on the accessor keyword in parse_property.rs and adds a new in-place lowering (lower_auto_accessors.rs, ~260 lines) called from visit_class for classes not routed through the standard-decorator lowering. It also fixes Class::can_be_moved to treat static accessor initializers as side effects, adds a new flags::Property::IsLoweredAutoAccessor variant consumed by emit_decorator_metadata_for_prop, and makes four small helpers in lower_decorators.rs pub(crate). ~520 lines of tests cover snapshot output, runtime semantics, emitDecoratorMetadata, useDefineForClassFields: false, and a two-file Bun.build renamer collision.

Prior review

My earlier inline comment identified that carrying the decorators on the getter meant lower_class copied the [_computedKey = expr] assignment into the __legacyDecorateClassTS call, evaluating a computed key twice. Commit 460ddf4 moved ts_decorators/ts_metadata to the setter (whose key is the bare temp) and added the PropertyKind::Set if IsLoweredAutoAccessor metadata arm; the new "legacy decorators on accessors receive the property descriptor" runtime test now asserts keyEvaluations: 1 with the decorator receiving key k0 and a get/set descriptor. Commit 3a37487 trimmed the flagged long comments. Both concerns are resolved.

Security risks

None. This is a syntactic transform in the transpiler; no I/O, network, permissions or untrusted-length arithmetic is involved. The new pass allocates only in the parser arena.

Level of scrutiny

High. The transpiler is production-critical and this introduces a new lowering that must compose correctly with three other passes (lower_class legacy decorators, the useDefineForClassFields: false constructor-relocation block, and the bundler's top-level renamer). The private-name collision avoidance walks the scope chain and the computed-key temp is registered in declared_symbols with an is_top_level bit — both are subtle enough that a maintainer familiar with the renamer should confirm the approach.

Other factors

Test coverage is thorough and each edge (private-name collision with an enclosing class, decorated computed-key accessor, metadata shape vs a plain getter, bundler cross-file naming) has both a snapshot and a runtime assertion. The PR description states the standard-decorator path is byte-identical before/after and that the listed test suites pass. Related open PRs #31926 and #35708 touch adjacent code on the standard-decorator side; the author has explained the non-overlap, but a maintainer may want to weigh in on the shared-implementation direction.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The double-evaluation of computed keys I flagged earlier is fixed in 460ddf4 (decorators/metadata now ride on the setter, whose key is the bare temp), and the new snapshot + keyEvaluations: 1 runtime test cover it. No further issues found this pass.

Given this adds a new ~260-line lowering pass that composes with lower_class, useDefineForClassFields, and the bundler renamer — and deliberately mirrors the desugaring shape of #31926 for later unification — a maintainer sign-off on the placement and the private-name/temp-ref scheme would still be worthwhile.

What was reviewed

  • Computed-key single-evaluation with legacy decorators — setter carries ts_decorators, decorate call gets the bare temp; runtime test asserts keyEvaluations: 1.
  • declare_var_temp_ref registers the hoisted temp in declared_symbols with is_top_level matched to the hoist scope, so the bundler renamer sees it (covered by the two-file Bun.build test).
  • Backing-name collision walk uses is_declared_in_enclosing_scopes while the class-body scope is current, so both this class's and enclosing classes' private names are avoided.
  • emit_decorator_metadata_for_prop arm matches Get | Set with IsLoweredAutoAccessor, so metadata emits once (on the setter) as design:type only.
Extended reasoning...

Overview

The PR unblocks accessor in TypeScript files that set experimentalDecorators/emitDecoratorMetadata, which previously failed to parse. It removes the standard_decorators gate in parse_property.rs, adds a new in-place lowering pass (lower_auto_accessors.rs, ~266 lines) invoked from visit_class for classes not routed through the standard-decorator lowering, adds an IsLoweredAutoAccessor property flag so emit_decorator_metadata_for_prop emits only design:type for the pair, fixes Class::can_be_moved to inspect static accessor initializers, and makes four lower_decorators.rs helpers pub(crate). ~600 lines of new tests cover snapshot output shapes and runtime semantics across the variant matrix (static/private/computed/literal keys, decorated + metadata, useDefineForClassFields: false, name collisions, bundling).

Security risks

None. This is a syntax-lowering change in the transpiler; no untrusted-input parsing beyond what the JS parser already does, no I/O, no auth/crypto.

Level of scrutiny

High. The JS parser/transpiler is a critical, correctness-sensitive path where a wrong lowering silently changes program semantics for every user with the affected tsconfig. The new pass has non-local interactions: it runs before the useDefineForClassFields: false block so generated backing fields get relocated, it hands legacy decorators to lower_class via the setter so the decorate call reuses the bare computed-key temp, and it registers hoisted var temps in declared_symbols so the bundler's cross-file renamer sees them. Each of these is tested, but the design (in-place lowering here vs. inside lower_decorators.rs, and the intended future unification with #31926) is a call a maintainer should confirm.

Other factors

My earlier review found that @dec accessor [expr] evaluated expr twice because decorators were carried on the getter (whose key is [_tmp = expr]). Commit 460ddf4 moved them to the setter, and the PR now has both a snapshot test showing __legacyDecorateClassTS([dec], A.prototype, __bun_temp_ref_1$, null) and a runtime test asserting keyEvaluations: 1 with the decorator receiving the correct descriptor. The comment-cop feedback was addressed in 3a37487 (comments trimmed). The bug-hunting pass on the current head found nothing. Test coverage is thorough and follows harness conventions (tempDir, bunEnv, drained pipes, test.concurrent for subprocess tests, exit-code asserted last). Given the scope and the overlap with two other open PRs implementing the same desugaring on the standard-decorator path, deferring for a human look rather than auto-approving.

@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

CI is green on all lanes for the latest push and there are no open review findings. Ready for a maintainer to look at the placement of the new lowering pass and the private-name/temp-ref scheme, per the review above.

Comment thread src/js_parser/parse/parse_property.rs Outdated
Comment thread src/js_parser/parse/parse_property.rs Outdated
Comment thread src/js_parser/parse/parse_property.rs
Comment thread src/jsc/RuntimeTranspilerCache.rs Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both earlier findings are addressed (460ddf4 moved the decorators/metadata to the setter so a decorated computed-key accessor evaluates its key once; 04aa1be gated the abstract-accessor widening on !standard_decorators so standard-mode output is unchanged), and this pass found nothing new. Given this adds a new lowering pass whose placement, private-name collision scheme, and declared_symbols registration for the bundler renamer you already flagged for a maintainer, a human look at those design points is still the right next step.

What was reviewed:

  • lower_auto_accessors_in_place: backing-field naming walks enclosing scopes, computed-key temp declared in the var-hoisting scope and registered top-level for the bundler renamer, decorators on the setter so lower_class reuses the bare temp key.
  • Interactions with useDefineForClassFields: false, emitDecoratorMetadata (IsLoweredAutoAccessor arm emits only design:type), and can_be_moved for static accessor initializers (cache version bumped).
  • Confirmed the pass is skipped for should_lower_standard_decorators classes and the abstract-accessor branch now only fires in legacy mode.
Extended reasoning...

Overview

The PR makes accessor parse in TypeScript files with experimentalDecorators/emitDecoratorMetadata and lowers it in place to #x; get x(){} set x(v){}, since JSC doesn't implement the keyword and the existing lowering only runs on the standard-decorator path. It adds a ~260-line pass (lower_auto_accessors.rs) called from visit_class, a new IsLoweredAutoAccessor property flag consumed by emit_decorator_metadata_for_prop and threaded through lower_class, widens can_be_moved to treat static accessor initializers as side effects (with a runtime-transpiler-cache version bump), and removes the standard_decorators gate on the accessor keyword in parse_property.rs. Four helpers in lower_decorators.rs are made pub(crate). ~600 lines of new snapshot + runtime tests cover key shapes, name collisions, decorated accessors, metadata, useDefineForClassFields: false, and a two-file bundle.

Security risks

None identified. This is transpiler output-shape work; inputs are source text already going through the parser, and the only new state introduced is arena-backed AST nodes and symbols. No untrusted length arithmetic, no filesystem/network, no user-overridable JS reached from native code.

Level of scrutiny

High. The JS parser/lowering is production-critical (every .ts file with experimentalDecorators now flows through the new pass), and the change sits at the intersection of several subtle subsystems: scope/symbol declaration during the visit pass, private-name printing (never renamed except by the minifier, so generated names must be unique by construction), the bundler's cross-file top-level renamer (the declared_symbols registration is load-bearing, as the bundling test demonstrates), and the legacy-decorator lowering in lower_class. The author explicitly asked for a maintainer to review the pass placement and the private-name/temp-ref scheme.

Other factors

  • Both concrete bugs I raised on earlier revisions have been fixed with tests pinning the behavior (the keyEvaluations: 1 runtime assertion for decorated computed-key accessors, and the standard-mode @dec abstract accessor snapshot in es-decorators.test.ts).
  • CI is reported green on all lanes for the latest push.
  • Two comment-cop nits remain open (on parse_property.rs:453 and RuntimeTranspilerCache.rs:56); the author has justified keeping both as one-line facts rather than workaround explanations, which reads reasonable to me.
  • Test coverage is thorough for the shapes described, but the design choices themselves (running the pass inside visit_class while the class-body scope is current, walking enclosing scopes for private-name avoidance, hoisting the computed-key var to the nearest hoisting scope and marking it top-level) are exactly the kind of thing a Bun parser maintainer should confirm matches how the rest of the lowering machinery expects symbols to be introduced.

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Green again on the latest head after the follow-ups (decorated abstract accessors in legacy mode only, standard-mode output re-verified byte-identical, transpiler cache version bump). All review threads are resolved, so this is ready for a maintainer look at the lowering pass placement and the private-name/temp-ref scheme.

@robobun

robobun commented Aug 14, 2026 •

Copy link
Copy Markdown
Collaborator Author

I independently wrote the same fix for this (branch farm/b5e844fc/accessor-experimental-decorators, commit ab11e8b: same parser change, same in-place expansion from visit_class, same hoisted temporary for computed keys). This PR is further along, so I am not opening a competing one. One finding from that branch that applies here as well:

nearest_stmt_list is None while visit_stmts preprocesses top-level TypeScript enums (the enum statements are visited before visit_stmts points nearest_stmt_list at its own before list), so lower_auto_accessors_in_place hits its expect("classes are only visited from within a statement list") on a computed-key accessor inside an enum member initializer. With experimentalDecorators in the tsconfig:

function k() { return "key"; }
enum E {
  A = (globalThis.K = class { accessor [k()] = 1 }, 0),
}
panic: classes are only visited from within a statement list
Crashed while visiting enum-edge.ts

The same window already bites the standard lowering on main: without the tsconfig, the file above fails with ReferenceError: _accessor_storage0 is not defined (a decorated class expression in that position fails with _dec is not defined), because lower_standard_decorators_expr silently drops its var declarations when there is no list to push them to.

The fix I ended up with is to move the prev_nearest_stmt_list save and the p.nearest_stmt_list = ...before assignment in visit_stmts up to right after before/after are created, so the enum preprocessing loop runs with it set:

bun/src/js_parser/visit/mod.rs

Lines 1317 to 1324 in ab11e8b

let prev_nearest_stmt_list = p.nearest_stmt_list;
// BACKREF — `before` outlives this block; raw NonNull avoids
// the `&'a mut` borrow conflict. Derive via `addr_of_mut!` (no intermediate
// `&mut`) so the pointer shares the local's base tag and survives the
// direct `&mut before` reborrows in the loops below (Stacked Borrows).
// Set before the enum preprocessing so declarations hoisted out of enum
// member initializers land in this list too.
p.nearest_stmt_list = NonNull::new(core::ptr::addr_of_mut!(before));
(the diff is git show ab11e8bba9 -- src/js_parser/visit/mod.rs). Tests for both modes are in that commit: decorators.test.ts "computed keys in classes nested in functions, parameters, static blocks and enums" and es-decorators.test.ts "class expressions inside enum initializers"; the second one fails on the released build with the ReferenceError above. Feel free to lift the hunk and the tests into this PR.

@robobun
robobun force-pushed the farm/ccdeb891/accessor-legacy-decorators branch from c5be06f to 00c18ae Compare August 26, 2026 05:38
Comment thread src/jsc/RuntimeTranspilerCache.rs Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed after the rebase and didn't find any new bugs — both earlier findings are fixed: the accessor's ts_decorators/ts_metadata now ride on the setter so @ dec accessor [expr] evaluates the key once, and the abstract accessor widening in parse_property.rs is gated on !standard_decorators with a covering test in es-decorators.test.ts. Given the scope (a new lowering pass plus parser/visitor/cache-version changes), a human pass is still worthwhile.

What was reviewed:

  • lower_auto_accessors.rs: decorators on the setter (202-203) paired with the Get | Set if IsLoweredAutoAccessor metadata arm in p.rs; private-name collision walk over generated names + enclosing scopes; computed-key temp registered in declared_symbols with is_top_level for the bundler renamer.
  • g.rs can_be_moved now treats static accessor initializers as side effects, and RuntimeTranspilerCache EXPECTED_VERSION bumped 26→27 to invalidate the older hoisted output.
  • parse_property.rs: accessor recognized in both decorator modes; decorated abstract accessor kept only in legacy mode, standard mode still drops it.
Extended reasoning...

Overview

This PR makes the parser accept the accessor class-member keyword under TypeScript's experimentalDecorators mode and adds a dedicated in-place lowering pass (src/js_parser/lower/lower_auto_accessors.rs, 266 lines) that desugars each accessor x = init into a private backing field plus a getter/setter pair. Supporting changes wire the pass into visit_class before the useDefineForClassFields lowering, add a flags::Property::IsLoweredAutoAccessor bit consumed by emit_decorator_metadata_for_prop (so only design:type is emitted, matching tsc), extend Class::can_be_moved to treat static accessor initializers as side effects, and bump the runtime transpiler cache version 26→27. Roughly 600 lines of transpiler-output and runtime-fixture tests were added across decorators.test.ts and es-decorators.test.ts.

Security risks

None identified. This is transpiler/AST-lowering logic operating on parsed source; there is no I/O, network, credential, or path handling. Allocations go through the parser arena via existing helpers (bump_name, BumpVec, StoreSlice); the cache-version bump is the correct format-version invalidation REVIEW.md requires for output-shape changes.

Level of scrutiny

High. This is a new lowering pass in the transpiler core with several interacting axes (static vs. instance, computed vs. literal vs. private keys, legacy decorator interaction, useDefineForClassFields, name collision across enclosing class scopes, bundler-renamer visibility of generated temps). The two concrete issues I raised on the previous push — computed-key double-evaluation when the decorators sat on the getter, and the abstract accessor widening leaking into standard-decorator mode — were both addressed exactly as suggested (commits 7fd7af80 and 4d87a374), and the cache bump landed in 00c18aee. This run's hunt exited on a dry streak with no new candidates.

Other factors

Test coverage is substantial and targeted: snapshot tests for each key/modifier variant, backing-name collision including an enclosing class, emitDecoratorMetadata shape vs. a real getter, a runtime fixture asserting a computed-key decorator's key function runs exactly once, a two-file Bun.build for renamer collisions on the generated _computedKey temp, and the static-accessor hoisting regression in both decorator modes. All prior third-party inline threads are bot-authored and self-resolved by the PR author; there are no outstanding human CHANGES_REQUESTED reviews. The change is well-shaped and well-tested but large enough, and central enough to the transpiler, that a maintainer should sign off rather than an automated approve.

…erimentalDecorators

The parser only recognized `accessor` when standard decorators were
enabled, so a .ts file in a project with experimentalDecorators (or
emitDecoratorMetadata) failed to parse a plain auto-accessor with
"Expected ";" but found ...". TypeScript accepts the keyword in every
decorator mode.

Parse the keyword regardless of the decorator mode and, for classes the
standard-decorator lowering does not process, desugar each accessor in
place into a private backing field plus a getter/setter pair. The
backing name avoids the private names declared by the class and by the
classes enclosing it; computed keys are evaluated once through a
hoisted temporary that is declared as a top-level symbol when the class
is top level, so the bundler's renamer keeps it apart from other files'
bindings. Legacy decorators on an accessor move to the generated getter,
which lower_class then decorates with the property descriptor and, with
emitDecoratorMetadata, the member's declared type, matching tsc.

Also stop hoisting classes whose static accessor initializers have side
effects: Class::can_be_moved only looked at plain static fields, so such
classes were moved above the bindings their initializers read.
…ted key is evaluated once

lower_class builds the __legacyDecorateClassTS call from the decorated
member's key. The getter's key is the assignment that evaluates a
computed key expression, so decorating through the getter ran the
expression a second time; the setter's key is the bare temporary.
@robobun
robobun force-pushed the farm/ccdeb891/accessor-legacy-decorators branch from 00c18ae to e9b7b98 Compare August 28, 2026 08:48
Comment thread src/jsc/RuntimeTranspilerCache.rs

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

@robobun

robobun commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

CI on the rebased head has one red test, test/js/web/url/url.test.ts on macOS x64. It is a pre-existing failure on main (the Unicode 16 IDNA table test) and this PR does not touch URL code. It is reported for main-break triage. Every other failure in build 107601 passed on retry. The decorator test suites pass on all lanes, so this stays ready for a maintainer.

@robobun

robobun commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

A maintainer asked for one change that covers every experimental decorator lowering bug in this area. #40830 does that, and it includes the fix this PR makes (decorated fields stay in the class body, computed keys are captured once, parameter decorators use the enclosing scope, decorators that read a private name run in a static block, export default @dec class, accessor lowering). If #40830 lands, this PR can be closed.

@robobun

robobun commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

#40830 fixes the experimentalDecorators lowering as a whole and also accepts and lowers the accessor keyword there. I built that branch and ran the tests of this PR against it.

What #40830 covers: the parse change in parse_property.rs, the in-place desugaring (#x_accessor_storage plus a getter/setter pair, in class statements and class expressions), decorated accessors receiving the descriptor with design:type only, computed keys evaluated once, and the #29197 repro. The runtime tests of this PR for those cases pass on #40830. The Bun.Transpiler snapshots differ only in the generated names (#x_accessor_storage, which is also the name tsc emits).

What this PR still fixes and #40830 does not:

  1. useDefineForClassFields: false. js_parser: fix TypeScript experimental decorator lowering #40830 lowers accessors in lower_class, after visit_class has moved the other instance fields into the constructor. The backing field keeps its initializer in the class body, so it runs before those assignments. For a = 1; accessor x = this.a + 1; b = 2; bun prints x: null and the order x, a, b. tsc emits this.#x_accessor_storage = this.a + 1 in the constructor, in source order (a, x, b, x: 2). This PR lowers in visit_class before that block, so the backing field is relocated with the other fields.
  2. Class::can_be_moved (src/ast/g.rs). A class whose only side effect is a static accessor initializer is still hoisted above the bindings it reads. const names = ["p"]; class S { static accessor s = names[0] } fails with ReferenceError: Cannot access 'names' before initialization, in .js files too.
  3. @dec abstract accessor g: number under experimentalDecorators. tsc emits __decorate([dec, __metadata("design:type", Number)], Entity.prototype, "g", void 0). js_parser: fix TypeScript experimental decorator lowering #40830 drops the member together with its decorators.
  4. The generated storage name only avoids the private names of the same class. With #x_accessor_storage declared in an enclosing class and read from the nested class body, the nested accessor shadows it and the read throws TypeError: Cannot access invalid private field. tsc emits #x_2_accessor_storage in that case.

So this PR stays open. Once #40830 lands, rebase this branch onto it. The parse change and the lower_class accessor lowering of #40830 then overlap with this branch. Keep the visit_class lowering from here (it is what fixes case 1), the can_be_moved change, and the abstract accessor handling. Adopting the _accessor_storage suffix keeps the output close to tsc.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Decorators on accessor class fields fail to parse in Bun

1 participant